-
Notifications
You must be signed in to change notification settings - Fork 844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Amsterdam] Updating shadows #3428
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
💚 CLA has been signed |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
b406fab
to
4e67af2
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick couple things on the aesthetics, and I'll be pushing up a PR for some code refactoring
@mixin euiSlightShadow($color: $euiShadowColor, $opacity: 0) { | ||
@include opacityWarning; | ||
box-shadow: | ||
0 .8px .8px rgba($color, shadowOpacity(.04)), | ||
0 2.3px 2px rgba($color, shadowOpacity(.03)), | ||
0 5.4px 5px rgba($color, shadowOpacity(.02)), | ||
0 18px 17px rgba($color, shadowOpacity(.01)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one that feels a little overkill to me. It's called slight
but has 4 layers with the largest being 18px
long. When I audit for usages of euiSlightShadow()
, I get:
- EuiButton/EuiButtonGroup: EUI only. Amsterdam removes the shadow
- EuiKeyPadMenuItem: Component itself hasn't been altered for Amsterdam yet and is still using the border style panel
- EuiStepHorizontal: With the color altered to blue, it's hardly visible at all and could probably just be removed
- form/euiCustomControl: These are all the form elements like checkboxes, radios, and radio style handles for sliders. These are all roughly
16x16
components, pretty small, so it's odd to have such a large shadow.
I'd suggest keeping the slight shadow very slight. It's possible once all those above components have been touched we may have no need for it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. I'll clean that up.
@function shadowOpacity($opacity) { | ||
@if (lightness($euiTextColor) < 50) { | ||
@return $opacity * 1; | ||
} @else { | ||
@return $opacity * 2.5; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
4e67af2
to
9869c6d
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
- Removed modal override and changed the default theme usage since it wasn’t really changing the opacity noticeabley anyway - Moved warning messages to each mixin … sigh - Made the docs layout look more Amsterdamy
04bd4dc
to
bbf5d15
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM! Shadows are really nice on things like cards and such.
Co-authored-by: Caroline Horn <[email protected]>
Jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3428/ |
Summary
Checklist